Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use old aggregation for 386 only, in 8.10 #11403

Merged
merged 7 commits into from
Aug 17, 2023
Merged

Use old aggregation for 386 only, in 8.10 #11403

merged 7 commits into from
Aug 17, 2023

Conversation

carsonip
Copy link
Member

@carsonip carsonip commented Aug 16, 2023 •
edited
Loading

Motivation/summary

Target branch: 8.10 (PR target branch needs to be updated once 8.10 branch is ready)
This is only for 8.10. Use old aggregation code for 386, and new LSM aggregation code otherwise.

Notes for 386 aggregation:

  • (Undocumented config) HDR histogram significant figures is no longer configurable
  • (Undocumented config) aggregation.transactions.max_services is moved to aggregation.max_services

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

  • Run system test for 386. Expect a few failures regarding new service.language.name in service destination metrics

To run system tests in 32 bit:

diff --git a/internal/beatcmd/cmd.go b/internal/beatcmd/cmd.go
index a312ebecd..9a8c2b08b 100644
--- a/internal/beatcmd/cmd.go
+++ b/internal/beatcmd/cmd.go
@@ -34,7 +34,7 @@ import (
 // NewRootCommand takes a BeatParams, which will be passed to
 // commands that must create an instance of APM Server.
 func NewRootCommand(beatParams BeatParams) *cobra.Command {
-       if err := platformcheck.CheckNativePlatformCompat(); err != nil {
+       if err := platformcheck.CheckNativePlatformCompat(); false && err != nil {
                fmt.Fprintf(os.Stderr, "Failed to initialize: %v\n", err)
                os.Exit(1)
        }
diff --git a/systemtest/apmservertest/command.go b/systemtest/apmservertest/command.go
index 8e6ebe8a2..b6bbc594e 100644
--- a/systemtest/apmservertest/command.go
+++ b/systemtest/apmservertest/command.go
@@ -34,7 +34,7 @@ import (
 // ServerCommand returns a ServerCmd (wrapping os/exec) for running
 // apm-server with args.
 func ServerCommand(ctx context.Context, subcommand string, args ...string) *ServerCmd {
-       binary, buildErr := BuildServerBinary(runtime.GOOS, runtime.GOARCH)
+       binary, buildErr := BuildServerBinary(runtime.GOOS, "386")
        if buildErr != nil {
                // Dummy command; Start etc. will return the build error.
                binary = "/usr/bin/false"

Related issues

Closes #11394

@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2023

This pull request does not have a backport label. Could you fix it @carsonip? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.x is the label to automatically backport to the 7.x branch.
  • backport-7./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Aug 16, 2023
@carsonip carsonip force-pushed the 8.10-386-use-old-aggregation branch from 4fad2e9 to 7ee1a9b Compare August 16, 2023 11:52
@carsonip carsonip force-pushed the 8.10-386-use-old-aggregation branch from 7ee1a9b to 754b190 Compare August 16, 2023 11:53
@carsonip carsonip force-pushed the 8.10-386-use-old-aggregation branch from 754b190 to ea8ddf0 Compare August 16, 2023 11:54
@carsonip carsonip force-pushed the 8.10-386-use-old-aggregation branch from ea8ddf0 to d3db658 Compare August 16, 2023 12:01
@carsonip carsonip changed the base branch from main to 8.10 August 16, 2023 14:56
@carsonip carsonip marked this pull request as ready for review August 16, 2023 15:51
@carsonip carsonip requested a review from a team as a code owner August 16, 2023 15:51
@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

This pull request is now in conflicts. Could you fix it @carsonip? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 8.10-386-use-old-aggregation upstream/8.10-386-use-old-aggregation
git merge upstream/8.10
git push upstream 8.10-386-use-old-aggregation

@carsonip
Copy link
Member Author

Closing as system tests are, and have been, failing on 32 bit systems regardless of this PR. We are revisiting the need to keep the old aggregation implementation for 386.

@carsonip carsonip closed this Aug 17, 2023
@carsonip carsonip reopened this Aug 17, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

This pull request is now in conflicts. Could you fix it @carsonip? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 8.10-386-use-old-aggregation upstream/8.10-386-use-old-aggregation
git merge upstream/8.10
git push upstream 8.10-386-use-old-aggregation

@carsonip carsonip merged commit cf83bd5 into elastic:8.10 Aug 17, 2023
8 checks passed
@carsonip carsonip mentioned this pull request Aug 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axw axw axw approved these changes

@kruskall kruskall kruskall approved these changes

Assignees
No one assigned
Labels
backport-skip Skip notification from the automated backport with mergify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

386 build failing after LSM PR merge
3 participants
@carsonip @axw @kruskall